feat(array): scalar probes with one-off and repeated probe types - #9843
Conversation
Merging this PR will degrade performance by 15.59%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | WallTime | words_gather_dispatch_avx512[1024] |
9 ns | 34 ns | -73.53% |
| ❌ | WallTime | arrow_checked_add_u32_neon[16384] |
13.5 µs | 20.3 µs | -33.42% |
| ❌ | Simulation | random_i8[0.5] |
71.3 µs | 94.9 µs | -24.81% |
| ❌ | Simulation | new_raw_prim_test_between[i32, 2048] |
64.1 µs | 77.6 µs | -17.43% |
| ❌ | Simulation | decompress[u64, (4000, 1024)] |
71.5 µs | 85.9 µs | -16.71% |
| ❌ | WallTime | filtered_sink_i64_neon[NineNullsInTen] |
15.4 µs | 17.4 µs | -11.42% |
| ❌ | WallTime | filtered_owned_i64_neon[NineNullsInTen] |
15.3 µs | 17 µs | -10.17% |
| ⚡ | Simulation | execute_scalar_struct_wide |
503 µs | 389.5 µs | +29.15% |
| ⚡ | Simulation | random_i16[0.8] |
96.4 µs | 78.5 µs | +22.83% |
| ⚡ | Simulation | execute_scalar_struct_simple |
106.5 µs | 89.6 µs | +18.8% |
| ⚡ | Simulation | allocate_drop_arrow[0] |
456.9 ns | 402.7 ns | +13.45% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ji/array-probe-api (22e0f50) with develop (b5f43ba)
Footnotes
-
218 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
a7c63a2 to
9ca43c4
Compare
|
I will leave one comment instead of mentioning the lines. I think the name churn is unfortunate but I think it's fine. The If we have ProbeUsage::Once we should replace execute_scalar with probe usage once invocation. The null checks are split and duplicated right now and this replacement would force us to unify it now. I think it's right that you want to let implementation decide. Not sure we need to hold arrayref in ProbeAccess::Once if we pass it to the function explicitly |
| /// | ||
| /// The default preserves the existing scalar path without adding caching. | ||
| fn probe_scalar<'a>( | ||
| array: ArrayView<'a, V>, |
There was a problem hiding this comment.
since you have this arrayview then ProbeAccess::Once state is weird but I could be missing something
|
This was not ready to review yet |
robert3005
left a comment
There was a problem hiding this comment.
this addresses all the comments I had. The only question I had is whether we want to defer validity check to the impl instead of having default BUT I think if we fix validity computation with this it's better
9f7c690 to
6f8afbe
Compare
…hell Adds the vtable hook the scalar probe API builds on, without any of the probe machinery. `OperationsVTable` gains `type ProbeState`, the state an encoding may keep across repeated reads, and `probe_scalar(state, index, ctx)`, defaulting to `scalar_at` so no encoding changes behaviour. `ProbeState<'_, V>` for now only carries the `ArrayView`; the erased dispatch routes one-off reads through it. Every encoding declares `type ProbeState = ()`. `scalar_at` stays until encodings migrate to `probe_scalar`. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CqrLKgPqFYGZK5sjk1qe7
Row access over arrays on top of the `probe_scalar` hook: - `ArrayProbe<'a>` is a bare `&ArrayRef` with no destructor. One-off reads, including every child read while recursing through nested arrays, own nothing, so there is no unwind cleanup, no out-of-line drop glue and no pinned temporary on the per-row path. This is what the CodSpeed regression on the earlier design came down to. - `RepeatedArrayProbe` owns its handle, encoding state, validity probe and child probes, created on first use and reused across rows. - Both implement `Probe`. `ArrayRef::probe()` and `ArrayRef::repeated_probe()` are the entry points; `execute_scalar` and `is_valid` are shims over `probe()`. - `ProbeState<'_, V>` carries the `ArrayView` and, for a repeated read, a borrow of the retained `RepeatedState`; it has no destructor. `state.array()`, `state.retained()`, `state.slot(i) -> impl Probe`, and the direct `child_scalar` / `child_is_valid`. Encodings never branch on policy for child reads. - `Validity::probe()` gives the retained validity accessor. - `Struct` and `Primitive` implement `probe_scalar`; `scalar_at` delegates through `ProbeState::once`. Not generic over the retention policy: a generic vtable method is instantiated in every crate that constructs arrays of the encoding, so the hot body compiles downstream without this crate's inlining; measured at 20-30 points on struct reads and rejected. The policy is a runtime `Option` check, folded into the one-off arm by `inline(always)` on the probe bodies. Local divan medians vs the develop tip, interleaved runs of 1000 samples: is_valid_per_element[1024] -10%, [256] -12%, execute_scalar_struct_wide -39%, execute_scalar_struct_simple -29%. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CqrLKgPqFYGZK5sjk1qe7
…tate - `ArrayProbe<'a>` is now an enum: `Once(&ArrayRef)` for a one-off read, `Repeated(&mut RepeatedArrayProbe)` for a borrow of a retained probe. Both variants are references, so it still has no destructor, which is the property the per-row numbers depend on; a compile-time `needs_drop` assertion pins that for `ArrayProbe` and `ProbeState`. - `ProbeState::slot(i) -> VortexResult<Option<ArrayProbe>>` is the child accessor: `None` for an absent slot, an error for an out-of-bounds one. `ProbeState::split()` returns the encoding state and a `ProbeChildren` as disjoint borrows, so a cache can stay borrowed while children are read. The `child_scalar` / `child_is_valid` shortcuts are gone; reading `Struct` fields through `slot()` measures within noise of them. - `ProbeState::once` is crate-private: it exists for the `scalar_at` delegation and goes away with `scalar_at`. - The `Probe` trait and the private `ChildProbe` enum are gone; the enum is the one borrowed reader and `RepeatedArrayProbe::as_probe` gives the retained variant where a probe is expected. - `ProbeValidity` and `Validity::probe` are gone: `RepeatedArrayProbe` resolves validity once into `uniform_validity: Option<bool>` or a boxed validity probe. - `ProbeStorage` is gone; the erased state slot is an `Option<Box<dyn Any>>` with a crate-private `repeated_state` accessor. - `RepeatedState` is crate-private, and the retained side lives in `probe/repeated.rs`. - No `inline(always)` anywhere: the per-row bodies are plain `#[inline]`. That leaves the one-off shim one thin frame above the encoding, which measures at parity with develop on `is_valid_per_element`; forcing the bodies inline was worth about 10% there and is deliberately not taken. Public probe items: `ArrayProbe`, `RepeatedArrayProbe`, `ProbeState`, `ProbeChildren`, and the `EncodingProbeState` alias. Local divan medians vs the develop tip, five interleaved runs of 1000 samples: is_valid_per_element at parity, execute_scalar_struct_wide about -34%, execute_scalar_struct_simple about -26%. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016CqrLKgPqFYGZK5sjk1qe7
d025a09 to
0fbdafe
Compare
`probe/array.rs` imports `OperationsVTable`, so the label resolves on its own and the explicit `crate::vtable::...` target duplicates it, tripping `rustdoc::redundant_explicit_links` under `-D warnings`. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FEKHRLFEn9Tc7mHcRG7Hc9
Adds reusable scalar probes with lazy state at each child slot. Repeated traversal of the same slot path reuses its preparation, including validity.
ArrayRef::execute_scalarandis_validnow go through a one-off probe with the same bounds and validity checks.Public API
ArrayProbe<'a>is the borrowed reader, eitherOnce(&ArrayRef)orRepeated(&mut RepeatedArrayProbe).RepeatedArrayProbeowns its array handle and can outlive the original handle; it keeps the encoding's state, its validity, and one retained probe per child slot, all created on first use and dropped with the probe.Encoding contract
Breaks
Need to add a
type ProbeState: Default + 'static;to theOperationsVTable